Restrict global_stats access to platform admins#1706
Conversation
📝 WalkthroughWalkthroughRestricts read access to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66390bc97b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,27 @@ | |||
| -- ============================================================================= | |||
There was a problem hiding this comment.
This migration uses the 20260226000000 version prefix, but that prefix is already used by 20260226000000_org_rls_require_self_2fa_update.sql; Supabase migration ordering/history is version-based, so this collision can make one migration unapplied or non-deterministic across environments, which risks leaving global_stats access policy changes out of sync.
Useful? React with 👍 / 👎.
| throws_ok ( | ||
| 'SELECT COUNT(*) FROM public.global_stats', | ||
| '42501', | ||
| 'permission denied', | ||
| 'Authenticated non-admin users should not be able to select from global_stats' |
There was a problem hiding this comment.
This assertion expects authenticated non-admin reads to raise 42501, but the migration grants SELECT to authenticated and enforces admin-only visibility via RLS, which yields an empty result set rather than a permission error; as written, this test will fail even when the policy is working as intended.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/migrations/20260227000000_restrict_global_stats_public_access.sql`:
- Around line 25-27: The grant/revoke setup on table public.global_stats creates
asymmetric behavior: anon lacks SELECT and gets error 42501 while authenticated
has SELECT but is blocked by the RLS policy and therefore returns an empty
result (not an error), causing Test 4b in the RLS scenarios to fail; fix by
either adjusting the test expectation for authenticated non-admin users in the
test named Test 4b of the RLS scenarios to assert is_empty() instead of
throws_ok(42501), or change the privilege setup in the migration
(public.global_stats) to revoke SELECT from authenticated entirely and grant
SELECT only to a specific role that admins have so the test can continue to
expect a 42501 permission denied error.
In `@supabase/tests/27_test_rls_scenarios.sql`:
- Around line 67-77: The test currently sets "SET LOCAL role TO authenticated"
and calls throws_ok('SELECT COUNT(*) FROM public.global_stats', ...) expecting
error 42501, but RLS grants SELECT and returns zero rows (no error) and
auth.uid() is NULL because no JWT claims are set; update the test to assert
empty results instead of expecting an error (replace throws_ok with an
emptiness/count check such as is_empty or assert count = 0 for the query 'SELECT
COUNT(*) FROM public.global_stats') and before running the query set the JWT
claims/user context so auth.uid() is populated (e.g., set appropriate jwt.claims
or session context for a non-admin user) to mirror the intended non-admin
scenario rather than relying only on SET LOCAL role TO authenticated.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
supabase/migrations/20260227000000_restrict_global_stats_public_access.sqlsupabase/tests/26_test_rls_policies.sqlsupabase/tests/27_test_rls_scenarios.sql
supabase/migrations/20260227000000_restrict_global_stats_public_access.sql
Show resolved
Hide resolved
…apgo into riderx/fix-global-stats
riderx
force-pushed
the
riderx/fix-global-stats
branch
from
9a79832 to
41e9db1
Compare
…apgo into riderx/fix-global-stats # Conflicts: # supabase/tests/27_test_rls_scenarios.sql
riderx
force-pushed
the
riderx/fix-global-stats
branch
from
0a29b66 to
1b6d45a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
supabase/tests/27_test_rls_scenarios.sql (1)
84-84: Tighten Test 4c comment wording for clarity.The comment is a bit ambiguous; consider making it direct and role-focused.
✏️ Suggested wording tweak
--- Test 4c: Non-admin can be replaced with admin and still query this table +++ Test 4c: Admin users should be able to query global_stats🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/tests/27_test_rls_scenarios.sql` at line 84, The comment for "Test 4c" is ambiguous; update the SQL comment text (the line starting with "-- Test 4c") to a clearer, role-focused sentence such as: "Test 4c: Ensure replacing a non-admin role with an admin role still allows the admin to query this table." Replace the existing ambiguous wording with that exact phrasing in the test comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@supabase/tests/27_test_rls_scenarios.sql`:
- Line 84: The comment for "Test 4c" is ambiguous; update the SQL comment text
(the line starting with "-- Test 4c") to a clearer, role-focused sentence such
as: "Test 4c: Ensure replacing a non-admin role with an admin role still allows
the admin to query this table." Replace the existing ambiguous wording with that
exact phrasing in the test comment.
|
1 participant
Summary (AI generated)
global_statsand restrictsSELECTto authenticated admins viapublic.is_admin(auth.uid()).supabase/tests/26_test_rls_policies.sqlto match the newAllow admin users to select global_statspolicy.supabase/tests/27_test_rls_scenarios.sqlto enforce denied access foranonand non-admin users, and allow access fortest_admin.Test plan (AI generated)
bun lint.bun lint:backend.Screenshots (AI generated)
Checklist (AI generated)
Summary by CodeRabbit
Access Control
Tests